Additional streams test consolidation gaps - #7182
Open
jasnell wants to merge 3 commits into
Open
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
Comment on lines
+285
to
+314
| // preventAbort AND preventCancel together on a starts-errored source: | ||
| // both suppressions hold and both ends stay un-shut-down. | ||
| export const preventAbortAndCancelCombo = { | ||
| async test() { | ||
| const err = new Error('src-err'); | ||
| let abortCalled = false; | ||
| const rs = new ReadableStream({ | ||
| start(c) { | ||
| c.error(err); | ||
| }, | ||
| }); | ||
| const ws = new WritableStream({ | ||
| abort() { | ||
| abortCalled = true; | ||
| }, | ||
| }); | ||
| strictEqual( | ||
| await rejectionOf( | ||
| rs.pipeTo(ws, { | ||
| preventAbort: true, | ||
| preventCancel: true, | ||
| preventClose: true, | ||
| }) | ||
| ), | ||
| err | ||
| ); | ||
| strictEqual(abortCalled, false); | ||
| ws.getWriter(); // dest untouched and re-lockable | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
A source error only takes the destination-abort branch; it never attempts to cancel the source. This test therefore cannot observe preventCancel, so a regression in that option passes while the comment claims both suppressions are covered. An abort signal initiates both shutdown actions.
Suggested change
| // preventAbort AND preventCancel together on a starts-errored source: | |
| // both suppressions hold and both ends stay un-shut-down. | |
| export const preventAbortAndCancelCombo = { | |
| async test() { | |
| const err = new Error('src-err'); | |
| let abortCalled = false; | |
| const rs = new ReadableStream({ | |
| start(c) { | |
| c.error(err); | |
| }, | |
| }); | |
| const ws = new WritableStream({ | |
| abort() { | |
| abortCalled = true; | |
| }, | |
| }); | |
| strictEqual( | |
| await rejectionOf( | |
| rs.pipeTo(ws, { | |
| preventAbort: true, | |
| preventCancel: true, | |
| preventClose: true, | |
| }) | |
| ), | |
| err | |
| ); | |
| strictEqual(abortCalled, false); | |
| ws.getWriter(); // dest untouched and re-lockable | |
| }, | |
| }; | |
| // An abort signal triggers both abort-destination and cancel-source | |
| // shutdown actions; preventAbort and preventCancel must suppress both. | |
| export const preventAbortAndCancelCombo = { | |
| async test() { | |
| const err = new Error('abort-reason'); | |
| const abortController = new AbortController(); | |
| let abortCalled = false; | |
| let cancelCalled = false; | |
| const rs = new ReadableStream({ | |
| cancel() { | |
| cancelCalled = true; | |
| }, | |
| }); | |
| const ws = new WritableStream({ | |
| abort() { | |
| abortCalled = true; | |
| }, | |
| }); | |
| const pipeP = rs.pipeTo(ws, { | |
| preventAbort: true, | |
| preventCancel: true, | |
| preventClose: true, | |
| signal: abortController.signal, | |
| }); | |
| await scheduler.wait(1); | |
| abortController.abort(err); | |
| strictEqual(await rejectionOf(pipeP), err); | |
| strictEqual(abortCalled, false); | |
| strictEqual(cancelCalled, false); | |
| strictEqual(rs.locked, false); | |
| ws.getWriter(); // dest untouched and re-lockable | |
| }, | |
| }; |
Comment on lines
+340
to
+348
| const pipeP = rs.pipeTo(ws); | ||
| controller.enqueue('chunk'); | ||
| await scheduler.wait(10); | ||
| controller.error(err); | ||
| await scheduler.wait(20); | ||
| // The write is still parked: abort must not have run yet. | ||
| strictEqual(events.join(','), 'write-start'); | ||
| releaseWrite(); | ||
| strictEqual(await rejectionOf(pipeP), err); |
Contributor
There was a problem hiding this comment.
This only observes that abort() has not run before the write is released. A broken pipe could reject before releaseWrite() while deferring the abort hook, and would still pass here. Track the pipe promise itself to verify that shutdown does not settle early.
Suggested change
| const pipeP = rs.pipeTo(ws); | |
| controller.enqueue('chunk'); | |
| await scheduler.wait(10); | |
| controller.error(err); | |
| await scheduler.wait(20); | |
| // The write is still parked: abort must not have run yet. | |
| strictEqual(events.join(','), 'write-start'); | |
| releaseWrite(); | |
| strictEqual(await rejectionOf(pipeP), err); | |
| const pipeP = rs.pipeTo(ws); | |
| let pipeSettled = false; | |
| pipeP.then( | |
| () => (pipeSettled = true), | |
| () => (pipeSettled = true) | |
| ); | |
| controller.enqueue('chunk'); | |
| await scheduler.wait(10); | |
| controller.error(err); | |
| await scheduler.wait(20); | |
| // The write is still parked: the pipe and its abort action must not settle yet. | |
| strictEqual(events.join(','), 'write-start'); | |
| strictEqual(pipeSettled, false); | |
| releaseWrite(); | |
| strictEqual(await rejectionOf(pipeP), err); |
Comment on lines
+492
to
+527
| export const abortThenControllerErrorInFlightWrite = { | ||
| async test() { | ||
| const events = []; | ||
| let rejectWrite; | ||
| let controller; | ||
| const ws = new WritableStream({ | ||
| start(c) { | ||
| controller = c; | ||
| }, | ||
| write() { | ||
| return new Promise((resolve, reject) => (rejectWrite = reject)); | ||
| }, | ||
| abort(reason) { | ||
| events.push(`sink-abort:${reason}`); | ||
| }, | ||
| }); | ||
| const writer = ws.getWriter(); | ||
| const write = writer.write('chunk'); | ||
| write.catch((e) => events.push(`write-rejected:${e.message}`)); | ||
| await scheduler.wait(1); | ||
| const abortP = writer.abort('abort-reason'); | ||
| abortP.then( | ||
| () => events.push('abort-fulfilled'), | ||
| (e) => events.push(`abort-rejected:${e.message}`) | ||
| ); | ||
| controller.error(new Error('controller-error')); | ||
| rejectWrite(new Error('write-failure')); | ||
| await scheduler.wait(20); | ||
| // PARITY: both implementations run the sink's abort hook EAGERLY, | ||
| // before the in-flight write settles, then surface the write | ||
| // rejection, then fulfill the abort. | ||
| strictEqual( | ||
| events.join(' | '), | ||
| 'sink-abort:abort-reason | write-rejected:write-failure | abort-fulfilled' | ||
| ); | ||
| }, |
Contributor
There was a problem hiding this comment.
The comment says this pins the stream's final error, but the test only checks callback order. A regression that changes writer.closed's rejection reason would pass.
Suggested change
| export const abortThenControllerErrorInFlightWrite = { | |
| async test() { | |
| const events = []; | |
| let rejectWrite; | |
| let controller; | |
| const ws = new WritableStream({ | |
| start(c) { | |
| controller = c; | |
| }, | |
| write() { | |
| return new Promise((resolve, reject) => (rejectWrite = reject)); | |
| }, | |
| abort(reason) { | |
| events.push(`sink-abort:${reason}`); | |
| }, | |
| }); | |
| const writer = ws.getWriter(); | |
| const write = writer.write('chunk'); | |
| write.catch((e) => events.push(`write-rejected:${e.message}`)); | |
| await scheduler.wait(1); | |
| const abortP = writer.abort('abort-reason'); | |
| abortP.then( | |
| () => events.push('abort-fulfilled'), | |
| (e) => events.push(`abort-rejected:${e.message}`) | |
| ); | |
| controller.error(new Error('controller-error')); | |
| rejectWrite(new Error('write-failure')); | |
| await scheduler.wait(20); | |
| // PARITY: both implementations run the sink's abort hook EAGERLY, | |
| // before the in-flight write settles, then surface the write | |
| // rejection, then fulfill the abort. | |
| strictEqual( | |
| events.join(' | '), | |
| 'sink-abort:abort-reason | write-rejected:write-failure | abort-fulfilled' | |
| ); | |
| }, | |
| export const abortThenControllerErrorInFlightWrite = { | |
| async test() { | |
| const events = []; | |
| let rejectWrite; | |
| let controller; | |
| const ws = new WritableStream({ | |
| start(c) { | |
| controller = c; | |
| }, | |
| write() { | |
| return new Promise((resolve, reject) => (rejectWrite = reject)); | |
| }, | |
| abort(reason) { | |
| events.push(`sink-abort:${reason}`); | |
| }, | |
| }); | |
| const writer = ws.getWriter(); | |
| const closed = writer.closed; | |
| const write = writer.write('chunk'); | |
| write.catch((e) => events.push(`write-rejected:${e.message}`)); | |
| await scheduler.wait(1); | |
| const abortP = writer.abort('abort-reason'); | |
| abortP.then( | |
| () => events.push('abort-fulfilled'), | |
| (e) => events.push(`abort-rejected:${e.message}`) | |
| ); | |
| controller.error(new Error('controller-error')); | |
| rejectWrite(new Error('write-failure')); | |
| await scheduler.wait(20); | |
| // PARITY: both implementations run the sink's abort hook EAGERLY, | |
| // before the in-flight write settles, then surface the write | |
| // rejection, then fulfill the abort. | |
| strictEqual( | |
| events.join(' | '), | |
| 'sink-abort:abort-reason | write-rejected:write-failure | abort-fulfilled' | |
| ); | |
| await rejects(closed, (e) => e === 'abort-reason'); | |
| }, | |
| }; |
Contributor
|
I'm Bonk, and I've done a quick review of your PR. Adds streams regression coverage and suite documentation.
Posted three inline suggestion comments. Time for a pun! These tests need to get their assertions in stream. |
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 3, 2026 18:39
1e7a415 to
69d6a88
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 3, 2026 18:47
69d6a88 to
26396a9
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 3, 2026 22:00
26396a9 to
f0e8bc8
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 3, 2026 22:40
f0e8bc8 to
6d34416
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 00:05
6d34416 to
46a6db6
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 01:32
46a6db6 to
85dea2e
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 01:51
85dea2e to
729db00
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 03:03
729db00 to
aa05643
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 14:00
aa05643 to
183f521
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 18:13
183f521 to
a267e48
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jasnell/streams-test-consolidation-10 #7182 +/- ##
======================================================================
Coverage 37.28% 37.28%
======================================================================
Files 800 800
Lines 251331 251331
Branches 19998 19998
======================================================================
Hits 93718 93718
Misses 146266 146266
Partials 11347 11347 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 19:28
a267e48 to
925088e
Compare
jasnell
force-pushed
the
jasnell/streams-test-consolidation-11
branch
from
September 4, 2026 19:29
925088e to
19350d6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Filling more gaps